Skip to content

refactor!: simplify identity model; drop acting-user plumbing#32

Merged
mafredri merged 11 commits into
mainfrom
mathias/codagt-437-token-owner-and-readme
May 18, 2026
Merged

refactor!: simplify identity model; drop acting-user plumbing#32
mafredri merged 11 commits into
mainfrom
mathias/codagt-437-token-owner-and-readme

Conversation

@mafredri
Copy link
Copy Markdown
Member

@mafredri mafredri commented May 15, 2026

The action creates a Coder Agents chat against a GitHub issue or PR. The chats API binds chat ownership to whoever the coder-token authenticates as; there is no owner override. This PR simplifies the action around that contract and pushes trigger policy out to the workflow author.

What changed

  • Drop inputs github-user-id and coder-username. The chat owner is always the token holder regardless of these inputs, so the action no longer resolves a non-token user.
  • Output coder-username reports the token holder, read once from GET /api/v2/users/me.
  • Workflow author defines trigger policy via if:; the action does not gate. GitHub's secrets.*-on-forks rule already covers the load-bearing case for pull_request. README's Security model section ships three patterns for the broader trigger surfaces.
  • Drop the per-user reuse label (coder-agents-chat-action-user). Chats are reused by gh-target plus workflow name; workflows that want per-actor separation pass idempotency-key: ${{ github.actor }} themselves.
  • Idempotency is stored as the VALUE of a fixed coder-agents-chat-action-idempotency label key, not as the key itself. A sanitized idempotency-key input can no longer collide with an action-owned label key.
  • Validate github-url against github.com in a shared parseGithubItemURL helper. Non-github hosts and malformed paths are refused before any GitHub API call.
  • Failure comments wrap detail.message and chat.last_error in a 4-backtick fenced block. Control bytes are stripped, embedded 4+-backtick runs are downgraded, and the body is capped at DETAIL_BLOCK_MAX_CHARS (4000) chars.
  • Rewrite the README around the simpler model: one Security model section covering ownership, trigger gating, and indirect prompt injection.

Closes CODAGT-437
Closes CODAGT-394
Closes CODAGT-438

🤖 Authored by Coder Agents.

mafredri added 2 commits May 15, 2026 13:37
The chat owner on `POST /api/experimental/chats` is always the
`coder-token` holder; the API has no owner override. The action
previously framed `coder-username` as picking "the Coder user the
chat runs as", which is incorrect.

What changed:
- Add CoderClient.getAuthenticatedUser() backed by GET /api/v2/users/me.
- resolveCoderUsername falls back to users/me when no input is set and
  the trust gate returns no-signal (schedule, workflow_dispatch with
  no sender or actor, custom repository_dispatch chains). The gate
  still refuses untrusted triggers and does not fall through to
  users/me.
- Warn when the resolved acting user (from coder-username or
  github-user-id) differs from the token owner. Suppressed for
  auto-resolved sources.
- Rewrite README "Security model" to make clear the trust gate
  protects the acting user (org pick, reuse label), not the chat
  owner. The chat owner is the token holder.
- generateChatUrl and buildDeploymentChatsUrl now build /agents and
  /agents/<id> paths, matching the real Coder frontend routes
  (site/src/pages/AgentsPage/utils/navigation.ts).
- Update inline docs on coder-username and github-user-id inputs and
  outputs to stop calling them "the chat runs as".

Closes CODAGT-437
Closes CODAGT-394
Closes CODAGT-438

🤖 Authored by Coder Agents.
…-github-user-id

The `acting-` prefix is the unambiguous signal that these inputs
select the acting user (org pick, per-user reuse label), not the chat
owner. The chat owner is the `coder-token` holder and is not
overridable.

What changed:
- Rename input `coder-username` -> `acting-coder-username` in action.yaml.
- Rename input `github-user-id` -> `acting-github-user-id` in action.yaml.
- Rename output `coder-username` -> `acting-coder-username` in action.yaml.
- Update getInput and setOutput calls; update OUTPUT_MAP entry.
- Update error message references, core.info lines, IdentitySource
  literal values, and failure-comment bodies.
- README inputs table, outputs table, identity resolution narrative,
  troubleshooting table, security model, and the doc-check recipe
  yaml example all renamed.
- Test names and string-contains assertions on the input keys updated.
- New schemas.test.ts regex assertion pins the mutex error message
  shape after the rename.

TS-internal field names (coderUsername, githubUserID) and the chat
label key (coder-agents-chat-action-user) are unchanged.

🤖 Authored by Coder Agents.
Copy link
Copy Markdown
Member Author

/coder-agents-review

Copy link
Copy Markdown

@coder-agents-review coder-agents-review Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The identity-model correction is well-reasoned: the API has no owner override, the users/me fallback is safely gated (hostile triggers throw before reaching it), the URL fix matches the real frontend routes, and the divergence warning adds observability without blocking. 271 tests pass with good positive and negative assertions. Pariston: "I tried to build a case against this change and could not."

The central issue is that the PR's own error messages and one README paragraph re-teach the exact mental model the PR exists to correct. Three user-facing strings still say "the user the chat should run as" when the whole point is that these inputs don't control who the chat runs as. Similarly, buildDeploymentChatsUrl and its surrounding names were updated in implementation but not in identity.

1 P2, 4 P3, 1 P4, 3 Nit. The P2 is the error-message/doc language contradicting the corrected ownership model. The P3s are: stale test fixture URL, function/field naming mismatch, README "runs under" language, and an untested catch path. The remaining findings are minor.


src/schemas.test.ts:331

P3 [DEREM-1] Stale /chats/ URL fixture not migrated to /agents/. Every other test file migrated chatUrl fixtures from /chats/ to /agents/. This one was missed. Change to "https://coder.test/agents/990e8400-e29b-41d4-a716-446655440000". (Netero)

Verified: grep -c 'chatUrl.*chats' src/schemas.test.ts returns 1; all other test files show 0 remaining /chats/ in URL fixtures.

🤖

src/action.ts:1067

Nit [DEREM-7] The PR added Resolved acting Coder user: '${coderUsername}' (source: ${identitySource}) at line 1054. This line (Coder username: ${coderUsername}) repeats the same value without the "acting" qualifier or the source. Should have been removed as part of the rename. (Gon)

🤖

🤖 This review was automatically generated with Coder Agents.

Comment thread src/action.ts Outdated
Comment thread src/comment.ts Outdated
Comment thread README.md Outdated
Comment thread src/action.ts Outdated
Comment thread src/action.ts Outdated
Comment on lines +791 to +795
throw new Error(
"Refusing to auto-resolve a GitHub identity: " +
`${trust.reason}. ` +
"Set the `coder-username` input to a Coder username, or set " +
"`github-user-id` to the GitHub numeric user id of the user " +
"the chat should run as.",
`Failed to resolve the \`coder-token\` owner via GET /api/v2/users/me: ${describeError(err)}. ` +
"Set the `acting-coder-username` input to a Coder username, or set " +
"`acting-github-user-id` to the GitHub numeric user id of the user the " +
"chat should run as.",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P4 [DEREM-6] The users/me fallback throw doesn't attach { cause: err }, losing the structured CoderAPIError for classifyError. The explicit-input error paths (e.g. line 653) use new ActionFailureError(... { cause: err }), preserving the cause chain. The auto-resolve throws (sender at 735, actor at 758/771) and this new users/me throw use plain new Error(...) without cause.

classifyError sees a plain Error, not the original CoderAPIError, so it always classifies as api_error regardless of the upstream status code. The message is descriptive enough for humans, but the cause chain is invisible to programmatic consumers. (Hisoka)

Pre-existing pattern; fixing only here would create an inconsistency. Noting for a future cleanup pass. (Hisoka P4, Chopper Note)

🤖

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving as-is. The sender and actor sibling throws (lines 728-730, 753-755, 766-768) use the same plain new Error(...) pattern; fixing only the users/me site would create an inconsistency without addressing the underlying class of failure. A separate cleanup pass should switch all four to ActionFailureError(..., { cause: err }) together.

🤖 Posted using /amend-review skill via Coder Agents.

Comment thread src/action.ts Outdated
Comment thread src/action.ts Outdated
Addresses the round-1 review findings on PR #32. The PR's own error
messages and one README paragraph still carried the pre-rename framing
("the user the chat should run as"), and the deployment-list URL
identity ("chats") was not renamed when the URL migrated to
`/agents`.

What changed:
- Rewrite three error-message strings to match the README's "acting
  user (for org pick and the per-user reuse label)" framing
  (DEREM-2: action.ts trust-gate refusal, action.ts users/me failure,
  comment.ts user_ambiguous).
- Rename `buildDeploymentChatsUrl` -> `buildDeploymentAgentsUrl`,
  `chatsUrl` field on `FailureCommentContext` -> `agentsUrl`, "View
  chats in the Coder deployment" link text -> "View agents", and the
  surrounding comment (DEREM-3). Update the call site in action.ts and
  the comment.test.ts tests.
- Rewrite the README quickstart paragraph to stop using "runs under"
  language for the acting user (DEREM-4).
- Add a divergence catch-path test: when `getAuthenticatedUser`
  rejects, action.run completes, a `core.warning` is emitted naming
  the fetch failure, and createChat is still reached (DEREM-5).
- Add a `core.info` for the `no-signal` trust-gate verdict so an
  operator debugging identity resolution can see the gate ran and
  deferred (DEREM-8).
- Fix the stale doc comment on `resolveOrganizationID` that claimed
  the username lookup happens here (DEREM-9). The lookup now always
  happens in `resolveCoderUsername`.
- Migrate the missed `/chats/` URL fixture in schemas.test.ts:331 to
  `/agents/` (DEREM-1).
- Drop the duplicate `Coder username: ...` info log; the new
  `Resolved acting Coder user: ... (source: ...)` line above already
  covers it (DEREM-7).

DEREM-6 (P4) declined: the `users/me` throw uses the same plain
`new Error(...)` shape as the sender/actor sibling throws on
purpose. Fixing only this site would create an inconsistency; the
cleanup is best landed as a separate pass across all four throws.

🤖 Authored by Coder Agents.
Copy link
Copy Markdown
Member Author

In reply to DEREM-1 (P3, src/schemas.test.ts:331): #32 (review)

Updated to https://coder.test/agents/... to match the rest of the test fixtures.


In reply to DEREM-7 (Nit, src/action.ts:1067): #32 (review)

Removed the duplicate Coder username: ... line; the new Resolved acting Coder user: '...' (source: ...) line emitted earlier already covers it with strictly more information.

🤖 Posted using /amend-review skill via Coder Agents.

Copy link
Copy Markdown
Member Author

/coder-agents-review

Copy link
Copy Markdown

@coder-agents-review coder-agents-review Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 8 R1 fixes verified by the full panel. Error messages now match the corrected ownership model. The buildDeploymentAgentsUrl rename is complete. The warnOnTokenOwnerDivergence catch path is tested. The README no longer says "runs under." DEREM-6 (P4, { cause: err } missing on users/me throw) remains acknowledged; the pre-existing pattern across four sibling throw sites makes a single-site fix create asymmetry.

The users/me fallback is safely gated: untrusted triggers throw before reaching it, and the test suite pins this with negative assertions on mockGetAuthenticatedUser. The divergence warning is correctly scoped to explicit inputs. 272 tests pass. Pariston: "I tried to build a case against this change and could not."

1 new Nit below. 1 Nit dropped (scope creep: pre-existing function name).

🤖 This review was automatically generated with Coder Agents.

Comment thread src/action.ts Outdated
…losed

A security review of PR #32 found that the trust gate, the per-user
reuse label, and the `acting-*` identity inputs each protect a
different invariant. The orchestrator's earlier collapse treated them
as one and lost both (F2 and F3 in the review). This rewrites the
identity model so the gate stays top-level and fail-closed, the
reuse-label query is simplified safely, and the `acting-*` surface is
dropped.

What changed:
- Drop input `acting-coder-username` and input `acting-github-user-id`.
  The chats API binds chat ownership to the `coder-token` holder; the
  `acting-*` inputs only selected the org and the per-user reuse label,
  which post-rewrite the action no longer scopes by acting user.
- Rename output `acting-coder-username` -> `coder-username`. The value
  now always reports the token owner from `users/me`.
- Drop the sender/actor auto-resolve chain entirely. `users/me` is the
  only identity path.
- Drop `warnOnTokenOwnerDivergence`, `getTokenOwner`, the tokenOwner
  memoization, the `IdentitySource` type, the `describeError` helper,
  and the `getCoderUserByGitHubId` / `getCoderUserByUsername` methods
  from `CoderClient`.
- Move `classifyAutoResolveTrust` from inside `resolveCoderUsername`
  to a top-level `assertTrustedTrigger` call in `runInner`, executed
  before `users/me` and `createChat`. The gate is always-on; no input
  bypasses it. `untrusted` throws, `trusted` and `no-signal` log and
  proceed.
- Drop the per-user reuse label (`coder-agents-chat-action-user`).
  All chats are owned by the token holder so per-actor labelling
  added no isolation. Workflows that want per-actor separation pass
  `idempotency-key: ${{ github.actor }}` themselves.
- Restructure the idempotency label: the sanitized user value is now
  the VALUE of a fixed `coder-agents-chat-action-idempotency` key,
  not the key itself. User input can no longer collide with action-
  owned keys; the runtime `RESERVED_LABEL_KEYS` collision check is
  retained only as a defensive export for future keys.
- Validate `github-url` host in `parseGithubURL` (F6). The shared
  `parseGithubItemURL` helper anchors the regex to `github.com` and
  rejects non-github hosts before either the action's create-comment
  call or `deriveCommentKey`'s marker derivation.
- Wrap `detail.message` and `chat.last_error` in a 4-backtick fenced
  code block in failure comments (F9). The block strips C0 control
  bytes (keeping newline and tab), downgrades embedded 4+-backtick
  runs so the surrounding fence stays closable, and caps the message
  at 4000 chars.
- Rewrite the README: identity-resolution section collapses to one
  paragraph; security-model section documents ownership, trust-gate,
  and indirect prompt injection (F1) as three separate concerns; the
  `pull_request_target` recipe gates fork PRs via an `if:` on
  `head.repo.full_name`; inputs and outputs tables match the new
  surface.

Tests:
- Drop the entire `Identity resolution` and `Token-owner divergence`
  describe blocks (sender/actor auto-resolve and divergence-warning
  tests).
- Drop tests that exercised the now-removed `getCoderUserByGitHubId`,
  `getCoderUserByUsername`, and `parseGithubUserID` paths.
- Update reuse-label tests: no per-user label is emitted; the new
  fixed-key idempotency scheme is asserted in place of the old
  user-input-as-key scheme; the reserved-key collision test is
  flipped to assert that collision is no longer possible.
- Add a new `Trust gate (top-level, always-on)` describe block
  pinning fork-PR refusal, NONE-association refusal, trusted-MEMBER
  acceptance, no-signal acceptance, and the absence of an input
  bypass.
- Add `parseGithubURL` tests for F6: non-github.com host refusal and
  attacker-redirect refusal.
- Add `renderDetailBlock` tests for F9: plain wrap, fence
  neutralization, control-byte stripping, length cap.
- Update the `deriveCommentKey` enterprise-URL test to assert the new
  raw-URL fallback (no parse on non-github hosts).

Closes CODAGT-437
Closes CODAGT-394
Closes CODAGT-438

🤖 Authored by Coder Agents.
@mafredri mafredri changed the title feat: derive chat owner from token, drop /chats path refactor!: simplify identity model; drop acting-user plumbing May 18, 2026
mafredri added 2 commits May 18, 2026 10:07
The previous commit removed a single named import. Biome's format
check collapses 2-name imports onto one line. The local
`bun run format` should have caught this but only ran on the
file before the manual edit. CI caught it.

🤖 Authored by Coder Agents.
…se scope

The input description still referenced "the resolved Coder user" as
part of the default scope; that component was dropped when the
per-user reuse label went away. Rewrite to describe the current
scope (gh-target + workflow name) and point users at
`${{ github.actor }}` for per-actor separation.

🤖 Authored by Coder Agents.
Copy link
Copy Markdown
Member Author

/coder-agents-review

Copy link
Copy Markdown

@coder-agents-review coder-agents-review Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major rewrite since R2: the identity model was simplified to token-owner-only, the acting-* inputs were dropped, the trust gate moved to top-level with no input bypass, and new hardening was added (F6 host validation, F9 detail rendering, idempotency key restructure). The architectural decision is sound: collapsing acting-user plumbing eliminates a class of identity-confusion bugs. Security hardening (F6 host anchor, F9 strip-cap-sanitize, trust gate fail-closed) verified correct by 7 reviewers. 212 tests pass. Hisoka: "Boring code gets silence. This code held up."

The main issue is stale code from the removed getCoderUserByGitHubId/getCoderUserByUsername methods: the user_not_found and user_ambiguous error kinds, their comment-body templates (which reference inputs that never existed in action.yaml), and the chat-error-kind output description all survived the removal of the only methods that produced them. The dead code is in the shipped bundle and the templates reference phantom inputs. Fixing DEREM-13 (remove the dead branches) also fixes DEREM-12, DEREM-15, and DEREM-16.

1 P2, 3 P3, 6 Nit. 1 dropped (subsumed).


src/coder-client.ts:288

Nit [DEREM-15] Comment says the client "raises user_not_found and user_ambiguous" but the methods that raised those (getCoderUserByGitHubId, getCoderUserByUsername) were removed. getAuthenticatedUser uses the generic request() which never sets a kind. (Netero, Bisky, Mafu-san)

🤖

action.yaml:120

P3 [DEREM-16] chat-error-kind output description still lists user_not_found and user_ambiguous as possible values. The README was updated to drop them; action.yaml was not. Documents disagree. (Leorio)

🤖

src/comment.ts:116

Nit [DEREM-19] deriveCommentKey hand-extracts GITHUB_URL_REGEX groups (lines 116-120) instead of calling the new parseGithubItemURL helper defined in the same file (line 128). Two extraction sites for one regex, introduced in the same PR. (Robin, Zoro)

🤖

🤖 This review was automatically generated with Coder Agents.

Comment thread src/comment.ts Outdated
Comment thread src/comment.ts Outdated
Comment thread src/schemas.ts
Comment thread src/action.test.ts Outdated
Comment thread src/action.ts Outdated
Comment thread src/action.ts Outdated
Comment thread src/action.ts Outdated
…me sanitizedKey

Remove the user_not_found and user_ambiguous ChatErrorKind variants:
the action no longer resolves an acting Coder user, so these branches
were unreachable. Update the enum, the FailureDetail union, the
classifyError fallthrough, mapErrorCodeToKind (removed), the two
buildFailureCommentBody cases, the action.yaml output description,
and the tests that exercised the dead paths.

Rename sanitizedKey to sanitizedIdempotency in action.ts; with the
fixed label key, the variable is an idempotency VALUE, not a key.
Have deriveCommentKey go through parseGithubItemURL instead of
matching GITHUB_URL_REGEX directly so host validation stays in one
place. Fix two JSDoc/comment paragraphs that still mentioned the
removed identity-resolution path, and replace stray escaped
apostrophes in two JSDoc comments. Add a schema test for the
existing-chat-id + force-new-chat mutex.
Copy link
Copy Markdown
Member Author

In reply to DEREM-15 (Nit, src/coder-client.ts:288): #32 (review)

Comment removed alongside the enum entries it described (see DEREM-13). The client has no path that raises user_not_found or user_ambiguous anymore.


In reply to DEREM-16 (P3, action.yaml:120): #32 (review)

Updated chat-error-kind description to list only the live kinds: spend_exceeded, org_not_found, api_error, timeout.


In reply to DEREM-19 (Nit, src/comment.ts:116): #32 (review)

deriveCommentKey now calls parseGithubItemURL and reads .owner / .repo / .number instead of indexing match groups. Host validation stays in one place; the fall-back behavior (use the raw URL when parsing fails) is preserved.

🤖 Posted using /amend-review skill via Coder Agents.

Copy link
Copy Markdown
Member Author

/coder-agents-review

Copy link
Copy Markdown

@coder-agents-review coder-agents-review Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 10 R3 findings verified fixed. Dead user_not_found/user_ambiguous code removed. Schema test restored. Stale names and docs corrected. The cleanup commit is clean and proportional. 205 tests pass.

The remaining findings are hygiene items from the R3 rewrite that the fix commit didn't touch: stale "auto-resolve" naming (the concept was removed but the function name and 5 doc references survive), a dead CoderAPIError.kind param, and two trust-gate test gaps. Pariston: "I tried to build a case against this and could not. The problem is correctly understood, the solution is proportional."

3 P3, 1 P4, 7 Nit.


src/sanitize-label-key.ts:20

P3 [DEREM-23] RESERVED_LABEL_KEYS is exported but has no production consumer. The runtime collision guard in action.ts was removed when idempotency switched to the fixed-key scheme. The sole consumer is sanitize-label-key.test.ts, whose assertions are tautological (RESERVED_LABEL_KEYS is new Set(Object.values(ACTION_LABEL_KEYS)), so testing .has(v) for each value is a definitional truth). Remove the export and its tests, or add a comment explaining retention. (Netero)

🤖

src/action.ts:212

P3 [DEREM-25] Five "auto-resolve" references survive the removal of auto-resolve. The function classifyAutoResolveTrust (line 212) still carries the old name, and doc comments at lines 96, 114, 168, 183 reference the concept. The operator-facing fork-PR error string at line 229 says "auto-resolve refuses to bind identity," but the action no longer binds identity; the trust gate is a standalone refusal with no identity-resolution model behind it.

Rename to classifyTriggerTrust (or similar) and update the five doc/string sites. (Gon P3, Leorio P3)

🤖

src/coder-client.ts:297

Nit [DEREM-28] CoderAPIError.kind param and JSDoc describe a classification mechanism removed in this PR. No caller sets kind; classifyError no longer reads it. The param is dead weight on the constructor. (Hisoka, Pariston, Chopper)

🤖

src/sanitize-label-key.ts:25

Nit [DEREM-29] sanitizeLabelKey name, filename, and docstring say "key" but both call sites now produce label values (the sanitized idempotency input becomes the VALUE of the fixed ACTION_LABEL_KEYS.idempotency key). Three reviewers flagged this. (Mafuuu, Gon, Meruem)

🤖

🤖 This review was automatically generated with Coder Agents.

Comment thread src/action.test.ts Outdated
Comment thread src/action.test.ts Outdated
Comment thread src/comment.ts Outdated
@@ -16,13 +16,49 @@ type Octokit = ReturnType<typeof getOctokit>;

// Shared regex for GitHub issue and PR URLs. Used by `deriveCommentKey` and
// `parseGithubURL` so adding another path (e.g. `/discussions/`) is one edit.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit [DEREM-30] GITHUB_URL_REGEX comment names deriveCommentKey and parseGithubURL as consumers, but the actual consumer is parseGithubItemURL (which replaced both extraction sites). (Gon)

🤖

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote the comment to describe what the regex matches and unexported it; the only consumer is parseGithubItemURL in the same file, so there is no longer a reason to publish it. (deriveCommentKey now also reaches the regex through parseGithubItemURL.)

🤖 Posted using /amend-review skill via Coder Agents.

Comment thread src/comment.ts Outdated
Comment thread src/comment.ts Outdated
…scrub stale doc language

- Rename classifyAutoResolveTrust to classifyTriggerTrust; the action no
  longer auto-resolves anything. Update the four doc paragraphs that
  referenced the dead model, the fork error string, the call site, and
  the README reference. Replace 'resolved Coder user'/'resolved user'
  framing in comment.ts, action.yaml, action.ts JSDoc, and four
  action.test.ts sites; nothing resolves now, the chat owner is the
  token holder.
- Rename sanitizeLabelKey to sanitizeLabelToken (file moved with git
  mv). The platform regex applies to label keys and values; one call
  site uses the result as a label value, the other as a comment-marker
  key.
- Delete RESERVED_LABEL_KEYS export and its tautological tests. With
  the fixed-key idempotency scheme a value can never overwrite an
  action-owned key, so the runtime guard was already gone.
- Delete the kind discriminator on CoderAPIError; classifyError no
  longer reads it and no caller sets it.
- Extract DETAIL_BLOCK_MAX_CHARS for the failure-block 4000-char cap;
  function body and test both reference the named constant.
- Unexport GITHUB_URL_REGEX and rewrite the docstring; the regex is
  internal to parseGithubItemURL now, not consumed by deriveCommentKey
  or any other caller.
- Add two trust-gate tests: head.repo === null (deleted fork) and
  head.repo.full_name diverging from base.repo.full_name with
  fork === false. Both branches were code-only before.
- Drop the placeholder src/index.test.ts; bun test discovery does not
  need a stub file.
Copy link
Copy Markdown
Member Author

In reply to DEREM-23 (P3, src/sanitize-label-key.ts:20): #32 (review)

Deleted the RESERVED_LABEL_KEYS export and its two tautological tests. With the fixed-key idempotency scheme, the sanitized value can never overwrite an action-owned label key; the runtime guard was already removed.


In reply to DEREM-25 (P3, src/action.ts:212): #32 (review)

Renamed classifyAutoResolveTrust to classifyTriggerTrust. Updated the four surrounding doc paragraphs (TRUSTED_AUTHOR_ASSOCIATIONS, ActionContext, TrustClassification, the function JSDoc) to drop "auto-resolve" framing, shrunk the fork error string from "auto-resolve refuses to bind ..." to "the pull request is from a fork" (the throw site already prefixes "Refusing to act on an untrusted trigger:" and instructs the workflow to add an if: gate), and updated the README reference.


In reply to DEREM-28 (Nit, src/coder-client.ts:297): #32 (review)

Removed the kind?: ChatErrorKind constructor param from CoderAPIError and rewrote the class JSDoc to describe the preserved response body (used by classifyError for the spend-exceeded 409 shape) without the dead discriminator.


In reply to DEREM-29 (Nit, src/sanitize-label-key.ts:25): #32 (review)

Renamed sanitizeLabelKey to sanitizeLabelToken (file moved via git mv). The platform's label regex applies to keys and values identically; one call site uses the result as a label value, the other as a comment-marker key, so LabelToken covers both. Docstring rewritten accordingly.

🤖 Posted using /amend-review skill via Coder Agents.

Copy link
Copy Markdown
Member Author

/coder-agents-review

Copy link
Copy Markdown

@coder-agents-review coder-agents-review Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All R4 findings verified fixed. classifyTriggerTrust renamed from the stale classifyAutoResolveTrust. Trust gate tests now cover deleted-fork and full_name-divergence paths. Dead CoderAPIError.kind removed. sanitizeLabelToken renamed from sanitizeLabelKey. All stale comments and magic numbers addressed. The author also ran a bonus audit pass, scrubbing four more stale doc references and dropping the placeholder test.

32 findings across 5 rounds; 28 fixed, 2 dropped (1 subsumed, 1 scope creep), 1 accepted (code later removed), 1 code-removed. Zero open. Netero: no new findings on the final delta. 199 tests pass.

🤖 This review was automatically generated with Coder Agents.

…d docs

Drop F1, F6, F9 (security-review tags), DEREM-2, and CODAGT-290
references from doc comments, test descriptions/comments, and a
README heading. The technical justification stays; the
internal-only identifiers do not survive on main.
@mafredri mafredri requested a review from johnstcn May 18, 2026 12:29
The action no longer classifies trigger trust. GitHub's secrets-on-fork
rule already gates the load-bearing case for `pull_request`; workflows
that opt into broader trigger surfaces (`pull_request_target`,
`issue_comment`, `pull_request_review`, `pull_request_review_comment`)
restate policy with `if:` themselves. Empirical investigation showed
the gate never reached community fork PRs in practice.

- Delete classifyTriggerTrust, TrustClassification, TRUSTED_AUTHOR_
  ASSOCIATIONS, and ActionContext from src/action.ts.
- Delete assertTrustedTrigger and its runInner call site.
- Drop the context parameter from CoderAgentChatAction's constructor;
  every test call site loses its 4th positional arg.
- Delete the Trust gate describe block from src/action.test.ts (seven
  tests including DEREM-26 and DEREM-27).
- Delete createMockContext and its ActionContext import from
  src/test-helpers.ts.
- Rewrite the README "Security model" section as three subsections:
  Chat ownership, Trigger gating (with three `if:` patterns and a link
  to GitHub's events doc), Indirect prompt injection. Drop stale
  "trust gate" references in Quickstart, the doc-check recipe, and the
  troubleshooting table.
Copy link
Copy Markdown
Member Author

/coder-agents-review

Copy link
Copy Markdown

@coder-agents-review coder-agents-review Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trust gate removed (net -642 lines). The action no longer gates on untrusted triggers; trigger policy is the workflow author's responsibility via if: conditions. GitHub's secrets.*-on-forks rule covers the load-bearing case. The architectural decision is defensible: the trust gate protected an identity-resolution model that no longer exists, and the README now ships three trigger-gating patterns for workflow authors.

6/8 panel reviewers found no issues. Netero clean. Two findings below: a misclassified failure comment (pre-existing but now the only surviving instance) and vacuous placeholder tests.

2 P3, 2 Nit.


src/action.ts:498

P3 [DEREM-33] classifyError has no branch for ActionFailureError. When resolveOrganizationID throws ActionFailureError("org_not_found", ...), the comment body renders the api_error template ("An unexpected error occurred") instead of the org_not_found template ("The Coder user has no matching organization"). Same for timeout. The chat-error-kind OUTPUT is correct (from ActionFailureError.kind), so automated workflows work, but the GitHub comment and the output disagree.

The fix is one branch at the top of classifyError: if (err instanceof ActionFailureError) return { kind: err.kind, message: err.message };. Pre-existing gap, but this PR removed mapErrorCodeToKind and the kind-check branch, leaving this as the only surviving misclassification path. (Mafuuu)

🤖

🤖 This review was automatically generated with Coder Agents.

Comment thread src/action.test.ts Outdated
Comment thread src/action.test.ts Outdated
Comment thread src/coder-client.ts Outdated
`classifyError` runs in comment.ts and cannot import ActionFailureError
without a cycle, so an ActionFailureError reaching `handleFailure`
fell through the `err instanceof Error` branch and rendered the
api_error body even when the thrown kind was `org_not_found` or
`timeout`. The `chat-error-kind` output already came from
`ActionFailureError.kind`, so the comment disagreed with the output.

Pre-check ActionFailureError in `handleFailure` and pass the kind
through. `spend_exceeded` carries extra fields on the body variant
and is only produced by `classifyError`, never thrown directly, so
the narrow is exhaustive. New test pins the org_not_found body.

Also drop dead weight surfaced by the R5 review:

- Delete `warnUnwiredInputs` and its describe block. The method
  was a no-op and the two tests asserted nothing.
- Delete the duplicate `creates a chat under the token owner
  returned by users/me` test; it was a strict subset of
  `creates new chat successfully` above it.
- Delete `ChatErrorKindSchema` and `ChatErrorKind` from
  coder-client.ts. The canonical definition lives in schemas.ts;
  the duplicate was a sync hazard. `comment.ts` re-exports
  `ChatErrorKind` from schemas.ts now.
Copy link
Copy Markdown
Member Author

In reply to DEREM-33 (P3, src/action.ts:498): #32 (review)

Pre-check ActionFailureError in handleFailure and pass error.kind straight into the body. classifyError lives in comment.ts and can't import ActionFailureError without a cycle, so the narrow happens at the caller. Test pins the org_not_found body.

🤖 Posted using /amend-review skill via Coder Agents.

Copy link
Copy Markdown
Member Author

/coder-agents-review

Copy link
Copy Markdown

@coder-agents-review coder-agents-review Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All R6 findings verified fixed. handleFailure now pre-checks ActionFailureError and passes error.kind directly into the comment body (DEREM-33). Vacuous warnUnwiredInputs deleted (DEREM-34). Duplicate test removed (DEREM-35). Dead ChatErrorKindSchema export removed (DEREM-36). Netero clean. Panel clean (3 reviewers).

36 findings across 7 rounds; 32 fixed, 2 dropped (1 subsumed, 1 scope creep), 1 accepted (code later removed), 1 code-removed. Zero open.

🤖 This review was automatically generated with Coder Agents.

@mafredri mafredri marked this pull request as ready for review May 18, 2026 15:50
@mafredri mafredri merged commit 4a30625 into main May 18, 2026
1 check passed
@mafredri mafredri deleted the mathias/codagt-437-token-owner-and-readme branch May 18, 2026 15:50
mafredri added a commit to coder/coder that referenced this pull request May 18, 2026
…r pin

The action refactor in coder/agents-chat-action#32 drops the
coder-username / github-user-id inputs and the auto-resolve trust gate.
Chat owner is now always the coder-token holder. Remove the
coder-username input and add an explicit fork-PR gate on the job; on
pull_request, GitHub already withholds secrets from fork PRs, so this
makes intent visible rather than relying on the side effect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants